Parse parameter decorators#3015
Conversation
Extend parameter AST nodes to retain decorators, including explicit-this decorators on function signatures, without forcing a broad constructor API churn. Teach both parameter parsers to accept stacked decorators on regular, rest, explicit-this, constructor-property, function-type, and parenthesized-arrow parameters. Update the AST builder to serialize parameter decorators inline so parser fixtures can round-trip the new syntax faithfully. Add a focused parser fixture covering the preserved syntax surface before deferred validation is introduced.
Add a Program-owned validation pass that walks the final AST and reports TS1206 once per decorated parameter, using the full decorator-list span for the diagnostic range. Invoke that validation from compile after transforms have had their afterInitialize window, so transformers can still remove parameter decorators before any diagnostics are emitted. Add a dedicated compiler rejection fixture covering regular, rest, explicit-this, constructor-property, function-type, function-expression, and arrow-parameter cases.
Add a dedicated transform input containing the same invalid parameter-decorator forms exercised by the compiler rejection fixture. Introduce ESM and CommonJS afterInitialize transforms that walk the AST and strip parameter decorators, including explicit-this decorators on function signatures. Extend the transform test scripts to compile that input with the stripping transforms, proving no TS1206 diagnostics are emitted once transforms remove the decorators in time.
Replace the bespoke parameter-decorator validation pass in Program with a small validator built on top of a shared AST walker. The previous implementation worked, but it reimplemented a near-complete AST traversal inside program.ts for one feature. That made Program responsible for AST topology, duplicated traversal logic, and created exactly the kind of ad hoc pass review feedback called out: every AST shape change would have to remember to update this validator as well. This change separates three concerns more cleanly: - parser.ts records which source-level statements actually preserved parameter decorators while parsing - ast.ts owns subtree traversal through a generic NodeWalker utility - program.ts only performs the parameter-specific check after afterInitialize In other words, the parser decides what roots are relevant, the walker decides how to descend the tree, and the validator decides what to report. This is better because it keeps AST traversal knowledge in one place, removes a large one-off recursive pass from Program, and makes future AST validation/analysis passes much smaller if they need to inspect subtrees later. The validator still runs after afterInitialize so transforms can inspect or remove preserved parameter decorators first, and the tests continue to cover parser round-tripping, compiler rejection, and transform-time removal. Also add a namespace regression case to ensure preserved parameter decorators nested under a tracked source-level statement are still validated and can still be stripped by transforms.
Drop AST imports from src/program.ts that became unused after moving subtree traversal into NodeWalker. This fixes the lint warnings reported in CI and keeps the parameter decorator refactor clean.
Follow the existing AST convention that Range is the last constructor field and the last factory/helper parameter. This updates ParameterNode and Node.createParameter accordingly, and adjusts all call sites.
Account for explicit-this parameter decorators in the same way as parameter decorators: wire explicitThisDecorators through FunctionTypeNode and Node.createFunctionType, keep it adjacent to explicitThisType, and leave range as the last constructor/helper parameter. Update all call sites accordingly.
Remove the duplicate parameter-decorator serializer in ASTBuilder and fold the inline formatting case into serializeDecorator with an optional flag. This keeps decorator serialization in one place while preserving the spacing differences between declaration decorators and parameter decorators.
Rename the parser helper that consumes and reports decorators found after a parameter has already started from reportInvalidParameterDecorators to tryParseParameterDecorators, matching its behavior more closely and aligning with the review suggestion.
| } | ||
|
|
||
| /** Generic AST walker. */ | ||
| export abstract class NodeWalker { |
There was a problem hiding this comment.
Why is there a whole new AST node walker...
There was a problem hiding this comment.
Looks like you decided to pick up the baton. Not sure why NodeWalker is needed here. Feels like this could’ve been done without it.
If you want to keep working on this, the first thing to do is try to understand the internal architecture of at least the AssemblyScript parser. Claude can probably explain it better. Then try giving it a task to review the implementation in this PR and rewrite everything (except the tests), sticking to the existing code style and paradigms, with as little “creative liberty” as possible, basically minimize the diff as much as you can.
Also, I recently came across an interesting technical prompt for Claude by Andrej Karpathy I haven’t tried it yet. LLMs are pretty much useless for the kind of tasks I’m dealing with right now, but it seems like a decent starting point. Of course, ideally, you’d want to figure everything out yourself.
There was a problem hiding this comment.
I think the NodeWalker here - if needed at all - can be delegated into the transform itself, with only the bits and pieces added to the AS parser and compiler that are strictly necessary to support parameter decorators. By default, if no transform consumes these decorators, the compiler would reject them as unsupported as it does with other decorators. That's not the feature though. The feature is that now a transform can consume them.
There was a problem hiding this comment.
This is definitely going to take me some time (especially because of how new I am). Also do you recommend I install claude code, or just paste the relevant files into a claude chat and analyze from there feeding it more context as it needs? I am somewhat unfamiliar with claude code and i've heard that it will modify a lot of stuff you may not want it to.
EDIT: this command worked well for getting the entire diff and pasting it into Claude git diff main...HEAD | pbcopy, it also copies the diff to your clipboard on MacOS
There was a problem hiding this comment.
Main goal right now is to just understand everything.
There was a problem hiding this comment.
I think the NodeWalker here - if needed at all - can be delegated into the transform itself, with only the bits and pieces added to the AS parser and compiler that are strictly necessary to support parameter decorators. By default, if no transform consumes these decorators, the compiler would reject them as unsupported as it does with other decorators. That's not the feature though. The feature is that now a transform can consume them.
It would be ideal, at least in my mind, if your code could remain portable such that a math function written with @wgsl decorators could still be imported and used by AS.
| export abstract class NodeWalker { | ||
|
|
||
| /** Indicates whether walking has been stopped. */ | ||
| stopped: bool = false; |
There was a problem hiding this comment.
The problem is that this property is never set to true during the tree traversal. It is always false. No one outside is set it either. I just don’t understand what the point of all this is. The comments don’t offer any clear explanation either. It looks like some sort of AI artifact / hallucination. NodeWalker is just used as base Visitor-like class for ParameterDecoratorValidator which by itself looks like a highly questionable decision
|
Result of running command > assemblyscript@0.0.0 test:parser
> node --enable-source-maps tests/parser parameter-decorators
Testing parser/parameter-decorators.ts
diff OK
[ SUCCESS ]Result of running command > assemblyscript@0.0.0 test:compiler
> node --enable-source-maps --no-warnings tests/compiler parameter-decorators --noDiff
# compiler/parameter-decorators
- compile debug
ERROR TS1206: Decorators are not valid here.
:
1 │ function regular(@first value: i32): void {}
│ ~~~~~~
└─ in parameter-decorators.ts(1,18)
ERROR TS1206: Decorators are not valid here.
:
2 │ function rest(@rest ...values: i32[]): void {}
│ ~~~~~
└─ in parameter-decorators.ts(2,15)
ERROR TS1206: Decorators are not valid here.
:
3 │ function withthis(@self this: i32, value: i32): i32 { return this; }
│ ~~~~~
└─ in parameter-decorators.ts(3,19)
ERROR TS1206: Decorators are not valid here.
:
6 │ constructor(@field public value: i32) {}
│ ~~~~~~
└─ in parameter-decorators.ts(6,15)
ERROR TS1206: Decorators are not valid here.
:
7 │ method(@arg value: i32): void {}
│ ~~~~
└─ in parameter-decorators.ts(7,10)
ERROR TS1206: Decorators are not valid here.
:
10 │ type Callback = (@arg value: i32) => void;
│ ~~~~
└─ in parameter-decorators.ts(10,18)
ERROR TS1206: Decorators are not valid here.
:
11 │ const expression = function(@arg value: i32): void {};
│ ~~~~
└─ in parameter-decorators.ts(11,29)
ERROR TS1206: Decorators are not valid here.
:
12 │ const arrow = (@arg value: i32): void => {};
│ ~~~~
└─ in parameter-decorators.ts(12,16)
ERROR TS1206: Decorators are not valid here.
:
14 │ export function nested(@arg value: i32): void {}
│ ~~~~
└─ in parameter-decorators.ts(14,26)
ERROR AS102: User-defined: "EOF"
:
17 │ ERROR("EOF");
│ ~~~~~~~~~~~~
└─ in parameter-decorators.ts(17,1)
FAILURE 10 compile error(s)
---
10 compile error(s)
---
SKIPPED (104.258 ms)
- compare stderr
SUCCESS (0.195 ms)
Time: 115 ms
[ SUCCESS ]Result of running > assemblyscript@0.0.0 test:transform
> npm run test:transform:esm && npm run test:transform:cjs
> assemblyscript@0.0.0 test:transform:esm
> node bin/asc tests/compiler/empty --transform ./tests/transform/index.js --noEmit && node bin/asc tests/compiler/empty --transform ./tests/transform/simple.js --noEmit && node bin/asc tests/transform/parameter-decorators.ts --transform ./tests/transform/remove-parameter-decorators.js --noEmit
Transform loaded
- constructor
- afterParse
defer
resolve
defer
resolve
defer
resolve
complete
- afterInitialize
defer
resolve
- afterCompile
Simple transform loaded
- afterParse
- afterInitialize
- afterCompile
Parameter decorator removal transform loaded
- afterInitialize strip parameter decorators
> assemblyscript@0.0.0 test:transform:cjs
> node bin/asc tests/compiler/empty --transform ./tests/transform/cjs/index.js --noEmit && node bin/asc tests/compiler/empty --transform ./tests/transform/cjs/simple.js --noEmit && node bin/asc tests/transform/parameter-decorators.ts --transform ./tests/transform/cjs/remove-parameter-decorators.js --noEmit
CommonJS transform loaded
- constructor
- afterParse
- afterInitialize
- afterCompile
Simple CommonJS transform loaded
- afterParse
- afterInitialize
- afterCompile
CommonJS parameter decorator removal transform loaded
- afterInitialize strip parameter decorators |
|
Okay so after messing about I think I understand what needs done. Default behaviour of decorators in AS is to ignore them if they are invalid. For example I can do: @blabla
@interesting("")
@wgsl.vertex("main")
function initBoard(): void { }... and there is no error. The problem is that doing the following: @blabla
@interesting("")
@wgsl.vertex("main")
function initBoard(@first hello: i32): @location(0) i32 {
return 0;
}...will throw compile errors: ERROR TS1003: Identifier expected.
:
45 │ function initBoard(@first hello: i32): @location(0) i32 {
│ ~
└─ in sketch.ts(45,19)
FAILURE 1 parse error(s)
ERROR TS1110: Type expected.
:
45 │ function initBoard(hello: i32): @location(0) i32 {
│ ~
└─ in sketch.ts(45,33)
FAILURE 1 parse error(s)So we only need (as dcode put it) "the bits and pieces added to the AS parser and compiler that are strictly necessary to support parameter decorators." Yet somehow this PR seems to be doing a LOT more than that. |
I prefer 2, the latter case. @MaxGraey @dcodeIO Also, after research it seems the only changes needed are in ast.ts and parser.ts (and possibly src/extra/ast.ts). Does this sound correct? |
|
Also, we can add suppression flag for compiler to completely ignore decorator errors. But in this case transformer should validate it by itself |
|
@MaxGraey I was hoping for some clarification on something you mentioned
Current behavour I have implemented outputs these types of errors: ERROR TS1206: Decorators are not valid here.
:
1 │ function regular(@first value: i32): void {}
│ ~~~~~~
└─ in parameter-decorators.ts(1,18)
ERROR TS1206: Decorators are not valid here.
:
2 │ function rest(@rest ...values: i32[]): void {}
│ ~~~~~
└─ in parameter-decorators.ts(2,15)Should I implement a custom ASXXX error code or continue to use TS1206? If we want custom error codes then how should I go about doing that? Here is the test commands if you want to check the output for yourself: |
|
If there's a suitable error in TS, it's best to use it but TS transpiler is fairly conservative and generally only throws errors that are still in tc39/proposals. And I don't recall what the status is there for parameter decorators. |
| private validateParameterDecorators(): void { | ||
| let program = this.program; | ||
| if (program.parameterDecoratorsValidated) return; | ||
| program.parameterDecoratorsValidated = true; | ||
| for (let source of program.sources) { | ||
| let fts = source.decoratedFunctionTypes; | ||
| if (!fts) continue; | ||
| for (let i = 0, k = fts.length; i < k; ++i) { | ||
| let ft = fts[i]; | ||
| let thisDecorators = ft.explicitThisDecorators; | ||
| if (thisDecorators && thisDecorators.length > 0) { | ||
| this.error( | ||
| DiagnosticCode.Decorators_are_not_valid_here, | ||
| Range.join(thisDecorators[0].range, thisDecorators[thisDecorators.length - 1].range) | ||
| ); | ||
| } | ||
| let params = ft.parameters; | ||
| for (let j = 0, m = params.length; j < m; ++j) { | ||
| let decorators = params[j].decorators; | ||
| if (decorators && decorators.length > 0) { | ||
| this.error( | ||
| DiagnosticCode.Decorators_are_not_valid_here, | ||
| Range.join(decorators[0].range, decorators[decorators.length - 1].range) | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I don’t like that transformer validation happens in the compiler. If it’s not valid for upstream AS semantics, it’s better to move it to transform utils or add it to their pipeline automatically.
TS will not merge decorators soon, and even if it does, parameters are still unclear:
https://github.com/tc39/proposal-decorators/blob/master/EXTENSIONS.md#parameter-decorators-and-annotations
If param decorator semantics are not standardized there either, it feels wrong to validate this inside our compiler.
There was a problem hiding this comment.
Actually, I’d just pass the bare minimum of what needs to be fed into the transformer, and let the transformer itself parse, canon and validate the decorators via the AST. Especially considering that this feature is pretty niche in terms of general use.
There was a problem hiding this comment.
To be honest, I have no idea what I am doing 😅
Good to know about the decorators proposal for TS, I wasn't aware of it before 👍
There was a problem hiding this comment.
What do you mean by "add it to their pipeline automatically"?
There was a problem hiding this comment.
I mean extend transform's API by adding new validation entry point here between listFiles and afterParse and add validation hook to this entry point instead validate by itself. This would allow users to validate the code in the transformer themselves before compilation but after parsing
There was a problem hiding this comment.
Actually, I’d just pass the bare minimum of what needs to be fed into the transformer, and let the transformer itself parse, canon and validate the decorators via the AST. Especially considering that this feature is pretty niche in terms of general use.
Sorry, I needed to reload the page before I saw this. When you say "Transformer" what are you referring to? I'm assuming you mean the implementors who will want to make use of these decorators? Eg. the example transformers in /tests/transform? Still very new to all the verbiage being thrown around here, sorry about the extreme noobiness
There was a problem hiding this comment.
Oh unless you are referring to the AssemblyScript AST transformer...
There was a problem hiding this comment.
I needed to reload the page before I saw this. When you say "Transformer" what are you referring to? I'm assuming you mean the implementors who will want to make use of these decorators?
yes
There was a problem hiding this comment.
Or don't do this and just use afterParse hook only.
Simply put, in this case we have ambiguity. Should we fail because we can’t handle such decorators, or not fail and hope that the Transformer will take on this role? An additional validator interface's method for Transformer API would answer this question clearly.
I think I finally get what you meant. I made some notes:
- AST can be extended without breaking changes, this is perfectly fine and anything that is added will just be ignored by the compiler but at least transformer plugins can then make use of it. No need to validate and throw errors unless it is an AST parse error.
- Need to add a transformer hook so transformers can add their own validation. This way AssemblyScript can delegate validation to transformers instead of having to handle it internally, which doesn't make sense because AS shouldn't be in charge of worring about that in the first place.
Is this correct? Just a "👍" is fine for me, unless I am completely wrong 🤣
| Void: 48, | ||
| While: 49, | ||
| ClassDeclaration: 51, | ||
| EnumDeclaration: 52, | ||
| FieldDeclaration: 54, | ||
| FunctionDeclaration: 55, | ||
| InterfaceDeclaration: 57, | ||
| MethodDeclaration: 58, | ||
| NamespaceDeclaration: 59, | ||
| TypeDeclaration: 60, | ||
| VariableDeclaration: 61 | ||
| }; | ||
|
|
||
| const LiteralKind = { | ||
| Template: 3, | ||
| Array: 5, | ||
| Object: 6 | ||
| }; | ||
|
|
There was a problem hiding this comment.
I'd use the TS version of the transformer to handle all of this
There was a problem hiding this comment.
TS version of the transformer?
There was a problem hiding this comment.
Oh I see, write in in typescript so i can import these from assemblyscript directly, yep
…ipt directly (Done with Claude Code)
I still don't think this is entirely correct because decorators are still not properly implement in the AST but at least it removes all the duplicate types.
Not sure why it changed the root package.json other than because we changed from js to ts. This also might be incorrect but I'll leave it alone for now.
According to claude, here's what changed:
tests/transform/remove-parameter-decorators.ts (new, 18 lines) — replaces the deleted 330-line JS file. Uses import type { Program } from assemblyscript for proper typing, and reads directly from source.decoratedFunctionTypes instead of walking the entire AST. No hardcoded NodeKind numbers at all.
tests/transform/cjs/remove-parameter-decorators.js (15 lines, was 330) — same simplification: drops the full AST walk, uses source.decoratedFunctionTypes directly.
package.json:88 — ESM test now runs with --experimental-strip-types --no-warnings and points to the .ts file.
The reason both transforms shrank so dramatically: the old versions walked the entire AST because they had no other way to find parameters with decorators. Now that the parser tracks source.decoratedFunctionTypes directly, a transform can just iterate that list.
I noticed Josh Tenner, did a final attempt to fix his PR... I am not sure if it was actually correct or not but I am making a new PR based on that one. Mainly wanting to try and see if I can do my own review on these changes and introduce myself into contributing to the AS repository for the coming years. The final goal of all this is for me to write an AST transform for outputting WGSL code which
@decoratorson parameters will be useful for.Note
I am not requesting a PR review or expecting this to be merged any time soon (unless the maintainer's want to do that). Making this PR is helpful for me to keep up to date with changes on main branch and seeing the live diff.
Changes proposed in this pull request:Changes I would like to eventually arrive at in this pull request:
Allow checking for
@decoratorson parameters and return types using a helper (with least modifications to source code of AssemblyScript). So that a transform plugin can access them and output WGSL code with the same@decorators.Something akin to this:
(AssemblyScript)
(WGSL)
Afaik this code allows reading
@decoratorsfrom parameters using a helper function. I'll be doing my own research into this and trying to acquaint myself with AssemblyScript internals. I mainly wanted the PR so I can keep it up to date with main and see a live diff of what is different from main.NOTE: I don't use Codex or ClaudeCode, nor am I interested in using them (I don't even have them installed). I do make use of Claude chat (claude.ai/new) quite often though for research and smaller pieces of code of which I often heavily modify, as well as occasional Copilot inline editing provided by VS Code.
Related Issues:
#2497
#20
Testing commands: